feat add codex fast mode support#2
feat add codex fast mode support#2skyguan92 wants to merge 2 commits intosplit/provider-model-pickerfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the fast mode functionality to support multiple providers, primarily integrating Codex and introducing a serviceTier setting. The changes include the addition of provider-aware fast mode utilities, updates to the API client and shims, and UI adjustments across various components. Feedback identifies a race condition in the applyFastMode function where the modelUpdated flag is returned before the state update completes, redundant state management logic, and a security regression caused by the removal of sensitive value redaction in the status display.
| function applyFastMode(options: { | ||
| enable: boolean | ||
| provider: FastModeCommandProvider | ||
| targetKey: string | ||
| setAppState: (f: (prev: AppState) => AppState) => void | ||
| }): { modelUpdated: boolean } { | ||
| const { enable, provider, targetKey, setAppState } = options | ||
|
|
||
| if (provider === 'codex') { | ||
| setCodexFastModeSelection(targetKey, enable ? 'fast' : null) | ||
| setAppState(prev => ({ | ||
| ...prev, | ||
| fastMode: enable, | ||
| })) | ||
| return { modelUpdated: false } | ||
| } | ||
|
|
||
| if (provider !== 'firstParty') { | ||
| return { modelUpdated: false } | ||
| } | ||
|
|
||
| clearFastModeCooldown() | ||
| updateSettingsForSource('userSettings', { | ||
| fastMode: enable ? true : undefined | ||
| }); | ||
| fastMode: enable ? true : undefined, | ||
| }) | ||
|
|
||
| if (enable) { | ||
| let modelUpdated = false | ||
| setAppState(prev => { | ||
| // Only switch model if current model doesn't support fast mode | ||
| const needsModelSwitch = !isFastModeSupportedByModel(prev.mainLoopModel); | ||
| modelUpdated = !isFastModeSupportedByModel(prev.mainLoopModel) | ||
| return { | ||
| ...prev, | ||
| ...(needsModelSwitch ? { | ||
| mainLoopModel: getFastModeModel(), | ||
| mainLoopModelForSession: null | ||
| } : {}), | ||
| fastMode: true | ||
| }; | ||
| }); | ||
| } else { | ||
| setAppState(prev => ({ | ||
| ...prev, | ||
| fastMode: false | ||
| })); | ||
| ...(modelUpdated | ||
| ? { | ||
| mainLoopModel: getFastModeModel(), | ||
| mainLoopModelForSession: null, | ||
| } | ||
| : {}), | ||
| fastMode: true, | ||
| } | ||
| }) | ||
| return { modelUpdated } | ||
| } | ||
|
|
||
| setAppState(prev => ({ | ||
| ...prev, | ||
| fastMode: false, | ||
| })) | ||
| return { modelUpdated: false } | ||
| } |
There was a problem hiding this comment.
The modelUpdated flag is being returned before the setAppState callback has executed. Since setAppState is asynchronous in React, modelUpdated will always return its initial value (false), which means the confirmation message won't correctly reflect model switches when they occur.
Additionally, the function should determine if a model switch is needed based on the current model passed as an argument to ensure the result is available synchronously.
function applyFastMode(options: {
enable: boolean
provider: FastModeCommandProvider
targetKey: string
setAppState: (f: (prev: AppState) => AppState) => void
currentModel: string | null
}): { modelUpdated: boolean } {
const { enable, provider, targetKey, setAppState, currentModel } = options
if (provider === 'codex') {
setCodexFastModeSelection(targetKey, enable ? 'fast' : null)
setAppState(prev => ({
...prev,
fastMode: enable,
}))
return { modelUpdated: false }
}
if (provider !== 'firstParty') {
return { modelUpdated: false }
}
clearFastModeCooldown()
updateSettingsForSource('userSettings', {
fastMode: enable ? true : undefined,
})
if (enable) {
const modelUpdated = !isFastModeSupportedByModel(currentModel)
setAppState(prev => ({
...prev,
...(modelUpdated
? {
mainLoopModel: getFastModeModel(),
mainLoopModelForSession: null,
}
: {}),
fastMode: true,
}))
return { modelUpdated }
}
setAppState(prev => ({
...prev,
fastMode: false,
}))
return { modelUpdated: false }
}
| const { modelUpdated } = applyFastMode({ | ||
| enable: enableFastMode, | ||
| provider, | ||
| targetKey: providerSelectionTargetKey, | ||
| setAppState, | ||
| }) |
There was a problem hiding this comment.
Update the applyFastMode call to pass the current model to support the signature change and fix the asynchronous flag bug.
| const { modelUpdated } = applyFastMode({ | |
| enable: enableFastMode, | |
| provider, | |
| targetKey: providerSelectionTargetKey, | |
| setAppState, | |
| }) | |
| const { modelUpdated } = applyFastMode({ | |
| enable: enableFastMode, | |
| provider, | |
| targetKey: providerSelectionTargetKey, | |
| setAppState, | |
| currentModel: model, | |
| }) |
| applyFastMode({ | ||
| enable: false, | ||
| provider, | ||
| targetKey: providerSelectionTargetKey, | ||
| setAppState, | ||
| }) |
There was a problem hiding this comment.
Update the applyFastMode call to match the new signature.
applyFastMode({
enable: false,
provider,
targetKey: providerSelectionTargetKey,
setAppState,
currentModel: null,
})
| const { modelUpdated } = applyFastMode({ | ||
| enable: options.enable, | ||
| provider: options.provider, | ||
| targetKey: appState.providerSelectionTargetKey, | ||
| setAppState: options.setAppState, | ||
| }) |
There was a problem hiding this comment.
Update the applyFastMode call to pass the current model from appState.
const appState = options.getAppState()
const { modelUpdated } = applyFastMode({
enable: options.enable,
provider: options.provider,
targetKey: appState.providerSelectionTargetKey,
setAppState: options.setAppState,
currentModel: appState.mainLoopModel,
})
| if ( | ||
| provider === 'firstParty' && | ||
| !enableFastMode && | ||
| !isFastModeSupportedByModel(model) | ||
| ) { | ||
| setAppState((prev: AppState) => ({ | ||
| ...prev, | ||
| fastMode: false, | ||
| })) | ||
| } |
There was a problem hiding this comment.
This block is redundant. applyFastMode (called on line 165) already handles setting fastMode: false in the app state when disabling fast mode for the firstParty provider.
| properties.push({ | ||
| label: 'Model', | ||
| value: redactSecretValueForDisplay(modelDisplay, process.env) ?? modelDisplay | ||
| value: modelDisplay |
There was a problem hiding this comment.
The removal of redactSecretValueForDisplay is a potential security regression. This utility prevents sensitive information (like API keys) from being surfaced in the UI if they are accidentally used as model names in environment variables. It should be retained.
| value: modelDisplay | |
| value: redactSecretValueForDisplay(modelDisplay, process.env) ?? modelDisplay |
| properties.push({ | ||
| label: 'Model', | ||
| value: redactSecretValueForDisplay(modelDisplay, process.env) ?? modelDisplay | ||
| value: modelDisplay |
There was a problem hiding this comment.
The removal of redactSecretValueForDisplay is a potential security regression. This utility prevents sensitive information (like API keys) from being surfaced in the UI if they are accidentally used as model names in environment variables. It should be retained.
| value: modelDisplay | |
| value: redactSecretValueForDisplay(modelDisplay, process.env) ?? modelDisplay |
|
Pushed a follow-up in I did not thread a separate |
1 participant
Summary
Notes
split/provider-model-pickerTesting
env -i PATH=/Users/jguan/.bun/bin:/usr/bin:/bin HOME=$HOME TMPDIR=${TMPDIR:-/tmp} bun test ./src/services/api/codexShim.test.ts ./src/utils/model/codexDisplay.test.ts ./src/utils/model/providerModelSettings.test.ts ./src/commands/model/model.test.tsxenv -i PATH=/Users/jguan/.bun/bin:/usr/bin:/bin HOME=$HOME TMPDIR=${TMPDIR:-/tmp} bun run build